Skip to content

Wilma Responsive redesign - #460

Closed
milospp wants to merge 4 commits into
vivo-project:mainfrom
milospp:feature/wilma-improvement
Closed

Wilma Responsive redesign#460
milospp wants to merge 4 commits into
vivo-project:mainfrom
milospp:feature/wilma-improvement

Conversation

@milospp

@milospp milospp commented Apr 9, 2024

Copy link
Copy Markdown
Contributor

GitHub issue 3910
GitHub PR VIVO 3972

What does this pull request do?

Added bootstrap (only grid style) CSS and modified Wilma theme to leverage its responsive design feature and enhance the overall layout.

What's new?

Added bootstrap v5
Refactored nav tabs in Person, Org Unit page to use bootstrap component instead of jQuery
Refactored files locations

How should this be tested?

Open every page and test by resizing up to around 400px in width (Mobile device toolbar can be used in google chrome to simulate phone display)

Additional Notes:

Here is the video example

  • The Left window is VIVO with old Wilma theme
  • The middle window is a new, updated Wilma theme
  • The right window is Tenderfoot theme (just for comparison)

Video Example
https://youtu.be/9B0_cevrEeE

@balmas balmas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code and responsiveness looks good to me but see my comment in vivo-project/VIVO#3972 about a problem loading the temporal google charts

if ( groupName == "viewAll" ) {
processViewAllTab();
}
console.log("View all")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest removing console debugging print

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I removed it.

@balmas balmas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the chart issue is already fixed in main and should be resolved in the merge, I think that this looks good

@litvinovg litvinovg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use webjar dependency #467
Also I think boostrap3 files shouldn't be moved as they are out of scope for this PR (Wilma didn't use boostrap3 before).

@milospp

milospp commented May 27, 2024

Copy link
Copy Markdown
Contributor Author

I suggest to use webjar dependency #467 Also I think boostrap3 files shouldn't be moved as they are out of scope for this PR (Wilma didn't use boostrap3 before).

I agree that Bootstrap 3 location refactor is not related to the Wilma theme directly. I can revert those changes, but would it be okay to have Bootstrap 5 on this location:
Vitro/webapp/src/main/webapp/bootstrap-5.3.2
and Bootstrap 3 on this:
Vitro/webapp/src/main/webapp/js/bootstrap

(Bootstrap 3 files are stored inside js subfolder)

@litvinovg

Copy link
Copy Markdown
Member

I suggest to use webjar dependency #467 Also I think boostrap3 files shouldn't be moved as they are out of scope for this PR (Wilma didn't use boostrap3 before).

I agree that Bootstrap 3 location refactor is not related to the Wilma theme directly. I can revert those changes, but would it be okay to have Bootstrap 5 on this location: Vitro/webapp/src/main/webapp/bootstrap-5.3.2 and Bootstrap 3 on this: Vitro/webapp/src/main/webapp/js/bootstrap

(Bootstrap 3 files are stored inside js subfolder)

With webjar there would be no need to upload bootstrap js/css assets into the project on each update and the path would /webjars/bootstrap/ (without version), which is important for customized themes update as js/css paths wouldn't change on next update.
Also all assets would be in one place, not like this one.

litvinovg and others added 4 commits May 31, 2024 13:37
Added Bootstrap and updated CSS for Wilma theme to make it responsive (vivo-project#429)

* Updated wilma to bootstrap 5

* Moved bootstrap to webapp folder, changed property tabs to use bootstrap

* Fixed property group tabs

* Bugfix: multiple dom object with same id, individual tabs

* Removed duplicate bootstrap 5.3.2

* Removed unused js code in propertGroupControls

* Reimplemented tab state persistence after refresh feature

* Removed unnecessary code
@milospp
milospp force-pushed the feature/wilma-improvement branch from 58c6d71 to 3e89331 Compare June 5, 2024 13:57
@balmas

balmas commented Jun 10, 2024

Copy link
Copy Markdown

@milospp is this one ready for re-review now?

@gneissone
gneissone self-requested a review August 5, 2024 23:07

@gneissone gneissone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you.

@litvinovg
litvinovg self-requested a review August 13, 2024 09:24

@litvinovg litvinovg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This set of PRs not only modifies Wilma theme to have responsive design, but also introduces global dependency for shared components on specific version of Bootstrap which can't safely be done retrospectively.
So it breaks backward compatibility for customized themes
https://wiki.lyrasis.org/display/VIVODOC115x/Creating+a+custom+theme
I think we should find a better way to do that.

@milospp milospp closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants